API review

Reviewer: Radu

Question / concerns / comments

Disclaimer: My comments/issues are unordered, and I tend to jump from small to big things as I go through each file in the package. I tried to be as critical as possible in order to help push this thing through. Not all issues need to be addressed, and if some are harsher than needed, please just ignore them.

• The description of the package is linked to the notion of "forearm camera". We need to remove this, as the cameras are used elsewhere.
  • BG: Good point. Fixed.
• Not sure if Eric M. and David P. have anything to do with this package, so I would remove them from the authors list, and instead add comments in the code (if certain sections were taken from some other GPL/etc code)
  • BG: They are the Mindtribe guys who wrote all the library stuff. It is totally relevant to have them listed.
• I would revise the license of the package and make the library GPL explicitly if it has a GPL part, and the node BSD
  • BG: The node is linked against the GPL library so it has to be GPL also. We are going to try to get rid of list.h at some point.
• The TODO file should be removed from the release version of the package
  • BG: I removed it because the TODO items were done, but generally do not consider a TODO file to be a problem in a release.
• Makefile contains a comment and a hack (target alltwice)
  • BG: That hack has been unnecessary for a while now. Thanks for catching it.
• I am not a fan of the commented defines in build.h (#define LIBRARY/DEVICE_BUILD), especially since they are not documented
  • BG: They are there so that the same include can be used in the driver and in the camera firmware. The two source trees have diverged a bit, but I would be remerging them at some point, so leaving these in. Not part of the API so commenting not critical.
• Some defines in ipcam_packet.h are not documented (e.g., CAMERA_NAME_LEN)
  • BG: Not part of the API.

• line 293 in list.h

  • BG: Not part of the API. Will fix if and when it becomes a problem. Interface names really are short. The longest I have seen is 10 characters, and that really was for a special case.
• Some more documentation in mt9v.h would be nice to have
  • BG: Not part of the API.
• host_util.c, list.c, mt9v.c, and wge100lib.c do not have licensing information in the header
  • BG: Thanks, added BSD headers.
• mt9v.c has no Doxygen API documentation and still contains test code (commented, lines 219-241)
  • BG: Not part of the API. (Killed the test code as it ws obsolete.)

• wge100lib.c, line 435 and 469... I know goto works, but maybe we can rewrite that part? This reminds me of Djikstra's "Go To Statement Considered Harmful" article (http://www.ecn.purdue.edu/ParaMount/papers/dijkstra68goto.pdf) Sames goes for 120, 132, 142, 156, 167, 178, 194, 210, 352, 365, 384, 417, 628, 633, 639, 680, 685, 690, 987

  • BG: This point has already been infinitely debated and universal agreement will never be achieved. My policy is to use goto in two cases:
    • Doing a double (or more) break. Ways that avoid goto are usually less readable.

    • Cleaning up after an error. Having the same cleanup code repeated 15 times in a function is error prone as it becomes likely that you forget to update one. As a testimony to the validity of this case, it is widely used throughout the linux kernel, and is explicitely acknowledged in the CodingStyle guide.

• A bit more documentation needed in upload_mcs.cpp, set_name.cpp, set_calibration.cpp, reset_cam.cpp (+license), reconfigure_cam.cpp (+license), read_all_flash.cpp (+license), discover.cpp, check_flash.cpp (+license), access_register.cpp (+license)
  • BG: Documentation of functionality or of the code?

• There's 3 different nodes being built: wge100_camera_node, wge100_camera_node_new, and wge100_camera_node_new2. That reminds me of the way I used to archive my images from the camera (new1, new2, new3... ). Do we need all of them?

  • BG: This is scheduled for cleanup in the coming week. (The API on the wiki corresponds to how things will be at the end of the week.) This is the fast way of getting in API breaking changes when you don't have time to clean up the users right away...

• There's some clear TODOs in some of the above mentioned files left, including "doxygen mainpage". Also, the class FrameTimeFilter for example does not have Doxygen-like documentation, so that needs to be reformatted.

  • BG: Not part of the API. TODOs will get cleaned up as they get prioritized.

• imageThread () in FrameTimeFilter still has a lot of commented out code. I feel wary that the class is still in a testing phase or is it simply not cleaned? Same for a few more methods below.

  • BG: Only wge100_camera_node_new2 is still relevant, and that part of it has been cleaned up.
• class WGE100CameraNode has no documentation
  • BG: Not part of the API.

• class VideoModeTestFrameHandler has no documentation

  • BG: Not part of the API.

• In general, see http://www.ros.org/doc/api/wge100_camera/html/annotated.html - we need at least some brief descriptions for each of these classes.

  • BG: Only relevant when I am releasing a code API.

• Not enough unit tests for the library Though not sure how many of those a driver really needs.

  • BG: Gets good coverage from the node tests (though they are currently disabled). Since the library API is not released it doesn't make sense to add more.

Conclusion

I did not check the usability of the classes and methods proposed in the API, as I assumed that the resultant driver nodes are working fine. I think we need to address a few of the issues mentioned above before release. The wiki pages are fantastic btw, abundant in information, so maybe taking a bit of that back to the code in a Doxygen fashion might be good.

In terms of the API however, I think that we will probably need to refactorize it once we have at least a few more similar camera drivers, so that they would share similar methods/attributes. Until then, I suppose that if whatever we have works, it should be good enough.

• BG: There is actually remarkably little that ends up being sharable between camera drivers. I have been factoring out the overall driver state machine into driver_base, but so far haven't found much beyond that that can be shared. The camera ROS API is somewhat standardized so that it is easy to use a camera without knowing which camera it is. Support for things like exposure and gain haven't been added to the standardized part yet, though. A similar standardization of the C++ API would be possible, but would require a big time investment with relatively little use for us.